Unbind cudf::size_type from offsets used by list columns - #23607
Unbind cudf::size_type from offsets used by list columns#23607davidwendt wants to merge 4 commits into
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR standardizes list, string, JSON, and TDigest offsets on fixed-width ChangesFixed-width offset migration
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cpp/src/groupby/sort/group_collect.cu (1)
66-70: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winDeclare explicit return types on the changed device lambdas.
Both changed lambdas are passed to device algorithms. Their return types should be explicit to prevent type deduction from becoming part of the CUDA algorithm contract.
cpp/src/groupby/sort/group_collect.cu#L66-L70: declare the lambda return type assize_type.cpp/src/rolling/detail/rolling_collect_list.cu#L61-L65: declare the lambda return type asbool.As per coding guidelines, extended device lambdas passed to device algorithms must declare explicit return types.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/src/groupby/sort/group_collect.cu` around lines 66 - 70, Declare explicit return types on both changed device lambdas passed to device algorithms: in cpp/src/groupby/sort/group_collect.cu lines 66-70, make the lambda returning the null-count result return size_type; in cpp/src/rolling/detail/rolling_collect_list.cu lines 61-65, make the corresponding lambda return bool.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cpp/include/cudf/detail/sizes_to_offsets_iterator.cuh`:
- Around line 289-292: Update the final STRING-offset construction in
ngrams_tokenize to call cudf::strings::detail::make_offsets_child_column instead
of the generic sizes-to-offsets helper, preserving the configured dynamic offset
type and allowing large-string totals. Keep the existing tokenization and offset
values unchanged.
- Around line 321-322: Prevent oversized cudf::size_type values from wrapping
before validation: in the generic path around the device lambda in
sizes_to_offsets_iterator.cuh, keep the scan input wide and perform the checked
int32_t conversion only after validating against total_elements; in
cpp/include/cudf/lists/detail/scatter.cuh lines 234-239, replace the direct
lv->view().size() narrowing with a checked int32_t step and preserve the
existing range-validation behavior.
In `@cpp/src/groupby/sort/group_collect.cu`:
- Around line 88-94: Enforce checked INT32 range handling for LIST offsets at
all four sites: in cpp/src/groupby/sort/group_collect.cu lines 88-94, validate
every group_offsets value before copying into the INT32 offsets column; in
cpp/include/cudf_test/column_wrapper.hpp lines 1614-1629, validate cumulative
nested child sizes before int32_t conversion; in
cpp/include/cudf_test/column_wrapper.hpp lines 1669-1675, validate c->size()
before constructing the INT32 offsets column; and in
cpp/src/io/utilities/column_buffer.cpp lines 107-110, reject or safely
checked-convert oversized string offsets before creating the LIST column. Ensure
no conversion can wrap or produce invalid memory access.
In `@cpp/src/lists/combine/concatenate_rows.cu`:
- Around line 108-116: Guard every size_type-to-int32_t LIST offset conversion:
in cpp/src/lists/combine/concatenate_rows.cu lines 108-116, accumulate into a
wider temporary and reject cumulative child counts above INT32_MAX before the
exclusive scan; in cpp/src/lists/copying/concatenate.cu lines 66-72, accumulate
shift in a wide type, validate its range, and return an explicit int32_t from
the device transform; in cpp/src/io/json/host_tree_algorithms.cu line 216,
validate row_offsets before scattering and use an int32-compatible scan; in
cpp/src/lists/utilities.cu lines 35-38, ensure labels_to_offsets rejects
label-derived values outside the int32_t range.
In `@cpp/src/lists/copying/copying.cu`:
- Around line 43-45: Keep LIST offset storage explicitly int32_t at all affected
sites: in cpp/src/lists/copying/copying.cu lines 43-45, allocate out_offsets as
rmm::device_uvector<int32_t>; in
cpp/src/lists/combine/concatenate_list_elements.cu lines 50-52, create the
empty-inner-child fallback with int32_t zero offsets; and in
cpp/src/io/json/parser_features.cpp line 78, use a LIST-specific int32_t
zero-offset helper while leaving string offsets unchanged.
In `@cpp/src/lists/sequences.cu`:
- Line 159: Update the internal LIST offset pointer declarations in
tabulator::offsets, sequences_dispatcher::operator(), and
sequences_functor::invoke from size_type const* to int32_t const*. Preserve
size_type for row counts and element indices so LIST offsets remain independent
of cudf::size_type.
In `@java/src/main/native/src/ColumnViewJni.cu`:
- Around line 182-183: Update the generate_list_offsets implementation to read
the validated INT32 list_length input using int32_t iterators, replacing the
cudf::size_type begin/end iterator types while preserving the existing
offset-generation logic.
---
Outside diff comments:
In `@cpp/src/groupby/sort/group_collect.cu`:
- Around line 66-70: Declare explicit return types on both changed device
lambdas passed to device algorithms: in cpp/src/groupby/sort/group_collect.cu
lines 66-70, make the lambda returning the null-count result return size_type;
in cpp/src/rolling/detail/rolling_collect_list.cu lines 61-65, make the
corresponding lambda return bool.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 32ae0536-4e3a-478e-ba27-395feed032d0
📒 Files selected for processing (46)
cpp/include/cudf/column/column_factories.hppcpp/include/cudf/detail/sizes_to_offsets_iterator.cuhcpp/include/cudf/lists/detail/gather.cuhcpp/include/cudf/lists/detail/scatter.cuhcpp/include/cudf/lists/list_device_view.cuhcpp/include/cudf/lists/lists_column_device_view.cuhcpp/include/cudf/lists/lists_column_view.hppcpp/include/cudf/tdigest/tdigest_column_view.hppcpp/include/cudf_test/column_wrapper.hppcpp/src/groupby/groupby.cucpp/src/groupby/sort/group_collect.cucpp/src/groupby/sort/group_merge_lists.cucpp/src/hash/md5_hash.cucpp/src/io/json/host_tree_algorithms.cucpp/src/io/json/nested_json.hppcpp/src/io/json/parser_features.cppcpp/src/io/orc/stripe_enc.cucpp/src/io/orc/writer_impl.cucpp/src/io/statistics/statistics.cuhcpp/src/io/utilities/column_buffer.cppcpp/src/io/utilities/column_buffer_strings.cucpp/src/lists/combine/concatenate_list_elements.cucpp/src/lists/combine/concatenate_rows.cucpp/src/lists/copying/concatenate.cucpp/src/lists/copying/copying.cucpp/src/lists/copying/gather.cucpp/src/lists/copying/scatter_helper.cucpp/src/lists/dremel.cucpp/src/lists/extract.cucpp/src/lists/interleave_columns.cucpp/src/lists/lists_column_factories.cucpp/src/lists/lists_column_view.cucpp/src/lists/reverse.cucpp/src/lists/segmented_sort.cucpp/src/lists/sequences.cucpp/src/lists/stream_compaction/apply_boolean_mask.cucpp/src/lists/utilities.cucpp/src/rolling/detail/rolling_collect_list.cucpp/src/rolling/detail/rolling_collect_list.cuhcpp/src/rolling/detail/rolling_operators.cuhcpp/src/strings/convert/convert_lists.cucpp/src/strings/repeat_strings.cucpp/src/transform/row_bit_count.cucpp/tests/groupby/collect_list_tests.cppjava/src/main/native/src/ColumnViewJni.cppjava/src/main/native/src/ColumnViewJni.cu
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cpp/src/quantiles/tdigest/tdigest_util.cuh (1)
18-18: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInclude
<cstdint>directly in this header.This header now declares
int32_t, but it does not include<cstdint>. Add the direct include instead of relying on transitive includes.As per coding guidelines, include headers directly for every used symbol.
Proposed include
`#pragma` once +#include <cstdint> + `#include` <cudf/detail/iterator.cuh>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/src/quantiles/tdigest/tdigest_util.cuh` at line 18, Update the header containing the t-digest offsets declaration to directly include the standard <cstdint> header before using int32_t, without relying on transitive includes.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cpp/src/quantiles/tdigest/tdigest.cu`:
- Line 362: In cpp/src/quantiles/tdigest/tdigest.cu at lines 362-362, reject
input.size() == INT32_MAX before allocating input.size() + 1 offsets; for
non-empty output, compute input.size() * percentiles.size() in a widened type
and reject values above INT32_MAX before num_output_values, grid sizing, and
exclusive scan. In cpp/src/strings/search/find_multiple.cu at lines 72-73,
reject strings_count == INT32_MAX before evaluating strings_count + 1, while
preserving the existing product check for the final offset.
---
Nitpick comments:
In `@cpp/src/quantiles/tdigest/tdigest_util.cuh`:
- Line 18: Update the header containing the t-digest offsets declaration to
directly include the standard <cstdint> header before using int32_t, without
relying on transitive includes.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 09ad97ba-5310-4520-99d2-f94c3b951c60
📒 Files selected for processing (5)
cpp/src/quantiles/tdigest/tdigest.cucpp/src/quantiles/tdigest/tdigest_aggregation.cucpp/src/quantiles/tdigest/tdigest_util.cuhcpp/src/strings/search/find_multiple.cucpp/src/text/minhash.cu
PointKernel
left a comment
There was a problem hiding this comment.
Changes look good to me. For my own learning, @davidwendt, how did you find all the places where size_type was being misused as an offset type? Or was it mostly AI effort?
Description
In preparation for evaluating a change of
cudf::size_typefromint32_ttoint64_t, this PR decouples LIST column offsets fromsize_type.Per the Apache Arrow columnar format, the offsets buffer width is part of the type, not an implementation detail —
List<T>hasint32offsets andLargeList<T>hasint64offsets, and these are distinct types:and from format/Schema.fbs, on
LargeList:The same split appears in the C Data Interface format strings:
+lis a list with int32 offsets,+Lis a large list withint64offsets.libcudf has
type_id::LISTand noLARGE_LIST. So aLISTcolumn's offsets child isint32by definition of the type — not "int32because that happens to besize_typetoday."Many places in the code, however, spelled that type as
cudf::size_type, viatype_to_id<size_type>(),data<size_type>(),element<size_type>(),size_type const*, etc. Those uses are correct only by the coincidence thatsize_type == int32_t. Under a 64-bitsize_typethey would either allocate INT64 offsets children — producing columns cuDF cannot export to Arrow, and thatcontiguous_splitand the JNI layer would misinterpret — or reinterpret anint32buffer asint64. Both are silent data corruption rather than a compile error, which is what makes them worth flushing out ahead of anysize_typechange rather than during one.This PR replaces those with explicit
int32_t / type_id::INT32at LIST offsets creation and read sites.STRING offsets are deliberately untouched: they are already dynamically
int32/int64and are handled through theoffsetalator.Why this isn't a new constraint: The codebase already depends on 32-bit list offsets everywhere it is forced to be explicit.
This PR makes that existing, already-relied-upon invariant explicit at the sites that were spelling it as
size_type.Follow-on work would now allow support of
LARGE_LISTin libcudf without explicitly adding a new type_id by simply checking the column's child offset type much like howLARGE_STRINGis supported in interop today. Theoffsetalatorwould similarly be employed to read/write the offset values correctly without need a special dispatch for the column type.Checklist